Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralized file for messages #91

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

NimperX
Copy link

@NimperX NimperX commented Oct 3, 2020

Centralized file for messages #90

@ckiee ckiee requested a review from robertt October 3, 2020 16:26
Copy link
Contributor

@jellz jellz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to include : string in every declaration, it is inferred

Also, instead of

const message = 'text';
export { message };

you can just do

export const message = 'text';

Copy link
Collaborator

@ckiee ckiee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a way to store messages with templates too, not sure how yet but this can be discussed.

.gitignore Outdated Show resolved Hide resolved
src/modules/msg.ts Outdated Show resolved Hide resolved
@jellz
Copy link
Contributor

jellz commented Oct 5, 2020

We need a way to store messages with templates too, not sure how yet but this can be discussed.

We can just export functions instead of strings from the file

// template
export const templateMessage = (a: string) => `${a} is used here`;

// static
export const staticMessage = () => 'static message';

@ckiee
Copy link
Collaborator

ckiee commented Oct 5, 2020

We need a way to store messages with templates too, not sure how yet but this can be discussed.

We can just export functions instead of strings from the file

// template
export const templateMessage = (a: string) => `${a} is used here`;

// static
export const staticMessage = () => 'static message';

This won't allow us to use the Discord.js toString() overrides as TypeScript will be mad.
Additionally, it will be quite big as we have to have that whole thing for each message.

p.s. haven't slept for a while, gonna write in more detail tomorrow.

@NimperX
Copy link
Author

NimperX commented Oct 5, 2020

We need a way to store messages with templates too, not sure how yet but this can be discussed.

In that case I think using a message class would be a perfect solution.

export class Message {
	functionWithStaticMessage(): string { return 'some text'; }

	functionWithParameter(a: string): string { return 'value of a is' + a; }
}

@ckiee
Copy link
Collaborator

ckiee commented Oct 5, 2020

@robertt Thoughts?

Personally I don't really like this since you have to make an instance of the class and the static keyword on every method would probably just make this even longer.
I'd like some Rust/Python-ey solution that lets us just have a object of strings and a function to call to populate stuff like %s but that seems quite far fetched for TS.

@jellz
Copy link
Contributor

jellz commented Oct 6, 2020

Consider:

export const messages = {
    staticMessage: () => 'static message here',
    templateMessage: (a: string) => `this is a template, ${a}`
}

Short lines and simplicity.

@robertt
Copy link
Member

robertt commented Oct 6, 2020

@jellz 👍, would probably be worth putting it in an object, so for example:

const messages = {
    commands: {
      rep: {
           toSelf: () => ':x: you cannot send rep to yourself',
           success: (member: string) => `:ok_hand: successfully sent rep to ${member}`
      }  
    },
};

@NimperX NimperX requested a review from ckiee October 12, 2020 04:22
Copy link
Collaborator

@ckiee ckiee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other then the previous unresolved comment but we still need to figure out how to organize the actual messages nicely.

p.s. we should probably consider future i18n too

@cspotcode
Copy link

Anything blocking this from being merged?

@NimperX NimperX requested a review from ckiee February 15, 2021 05:48
Copy link
Collaborator

@ckiee ckiee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the review comments I'd also like to mention you should try to make the constant's names a bit more detached from the sentence.

E.g. beAskerToCloseChannel -> noChannelClosePerms / cannotSendRepToYou -> noSelfRep. (or something like that)

.github/workflows/main.yml Show resolved Hide resolved
src/modules/msg.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ckiee ckiee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants